Add CompStatus Events to CertAbuseProcessor BED-6967 #191

lrfalslev · 2025-12-15T19:02:19Z

Description

Update CertAbuseProcessor to write contacted machines to the compstatus log.

Motivation and Context

BED-6967

How Has This Been Tested?

Added new unit tests and manually tested SHCE and SHE in a lab.

Screenshots (if appropriate):

Types of changes

Chore (a change that does not modify the application functionality)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

Documentation updates are needed, and have been made accordingly.
I have added and/or updated tests to cover my changes.
All new and existing tests passed.
My changes include a database migration.

Summary by CodeRabbit

Refactor
- Centralized status reporting into an event-driven flow and reduced parameter chaining between components.
- Added explicit cleanup of event handlers after processing to avoid duplicate notifications and leaks.
Bug Fixes
- Removed stray formatting noise.
- Made status/progress emission paths more consistent and reliable.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-12-15T19:02:30Z

Walkthrough

Centralizes CSVComputerStatus reporting inside ObjectProcessors by adding a Channel to its constructor, removing per-call channel parameters from processing methods, wiring internal status events to forward to that channel, and ensuring callers call ClearEventHandlers() after processing.

Changes

Cohort / File(s)	Summary
ObjectProcessors core `src/Runtime/ObjectProcessors.cs`	Constructor now accepts `Channel<CSVComputerStatus> compStatusChannel`; wires processor-specific status events to `HandleCompStatusEvent`, replaces direct channel writes with event-driven emissions, adds `ClearEventHandlers()`, and removes channel parameters from many internal Process* method signatures.
Collection task updates `src/Runtime/CollectionTask.cs`	Instantiate `ObjectProcessors` with `_compStatusChannel`; update `ProcessObject(...)` call to new signature (channel arg removed); add `processor.ClearEventHandlers()`; minor whitespace fix in `StartCollection`.
LDAP consumer updates `src/Runtime/LDAPConsumer.cs`	Instantiate `ObjectProcessors(context, log, computerStatusChannel)`; call `processor.ProcessObject(item, res)` (no channel arg); call `processor.ClearEventHandlers()` after batch/exception paths.

Sequence Diagram(s)

mermaid
sequenceDiagram
autonumber
participant Caller as Caller\n(CollectionTask / LDAPConsumer)
participant ObjProc as ObjectProcessors
participant Proc as SpecificProcessor
participant Handler as HandleCompStatusEvent
participant Channel as CompStatusChannel
participant Writer as CSVWriter/Consumer
Note over Caller,ObjProc: Caller constructs ObjectProcessors with channel
Caller->>ObjProc: new ObjectProcessors(context, log, compStatusChannel)
Caller->>ObjProc: ProcessObject(item, res)
ObjProc->>Proc: invoke processor logic
Proc-->>ObjProc: emit ComputerStatusEvent(CSVComputerStatus)
ObjProc->>Handler: HandleCompStatusEvent(event)
Handler->>Channel: channel.Writer.TryWrite(CSVComputerStatus)
Channel->>Writer: CSVComputerStatus delivered

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Collect EnrollmentAgent Restrictions For AllTemplates, add CertAbuseProcessor Tests BED-7044 #192 — Directly related: updates ObjectProcessors to accept comp-status channel, removes channel param from ProcessObject, and adds ClearEventHandlers/event-based forwarding.
Adding ObjectId to CompStatus #179 — Modifies CSVComputerStatus population (ObjectId field); touches the same status emission structures.
Add CompStatus Writing Locations #161 — Earlier changes to comp-status channel wiring and propagation; overlaps with constructor/signature updates.

Suggested reviewers

ktstrader
definitelynotagoblin

Poem

🐰 I hopped through stacks of code so neat,

Routed statuses down a single street,
Handlers linked, then cleared with care,
Channels hum and statuses share,
A rabbit's cheer for tidy feat! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly aligns with the primary change: adding CompStatus event handling to CertAbuseProcessor as specified in BED-6967.
Description check	✅ Passed	The description provides clear details of the change, links to the tracking issue (BED-6967), documents testing approach, and indicates tests passed, fulfilling template requirements.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

🧹 Recent nitpick comments

src/Runtime/ObjectProcessors.cs (1)

769-843: Consider extracting host resolution to avoid duplicate work.

Both the CertServices block (lines 773-792) and CARegistry block (lines 803-816) independently resolve the host to SID and emit status events. When both collection methods are enabled:

ResolveHostToSid is called twice
Two identical status events are emitted for ProcessEnterpriseCA

Consider extracting the host resolution outside both blocks to run once and share the result.

♻️ Suggested refactor

         if (_methods.HasFlag(CollectionMethod.CertServices)) {
             var caName = entry.GetProperty(LDAPProperties.Name);
             var dnsHostName = entry.GetProperty(LDAPProperties.DNSHostName);

             if (caName != null && dnsHostName != null) {
-                if (await _context.LDAPUtils.ResolveHostToSid(dnsHostName, resolvedSearchResult.DomainSid) is
-                        (true, var sid) && sid.StartsWith("S-1-")) {
-                    ret.HostingComputer = sid;
-                    await HandleCompStatusEvent(new CSVComputerStatus
-                        {
-                            Status = ComputerStatus.Success,
-                            ComputerName = resolvedSearchResult.DisplayName,
-                            Task = nameof(ProcessEnterpriseCA),
-                            ObjectId = resolvedSearchResult.ObjectId,
-                        });
-                } else {
-                    _log.LogWarning("CA {Name} host ({Dns}) could not be resolved to a SID.", caName, dnsHostName);
-                }
+                // Host resolution moved earlier (before CertServices/CARegistry blocks)
                 var caEnrollmentProcessor = new CAEnrollmentProcessor(dnsHostName, caName, _log);

Move the common host resolution to execute once before both conditional blocks when either CertServices or CARegistry is enabled.

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6eb6525 and 179f88d.

📒 Files selected for processing (1)

src/Runtime/ObjectProcessors.cs

🔇 Additional comments (7)

src/Runtime/ObjectProcessors.cs (7)

16-16: LGTM!

Import added to support the new RegistryAccessor and SAMServerAccessor dependencies.

44-78: LGTM!

The centralized event-driven status reporting pattern is well implemented:

Channel dependency injected via constructor

All relevant processor events are subscribed to HandleCompStatusEvent

CertAbuseProcessor correctly wired with new RegistryAccessor and SAMServerAccessor dependencies

80-88: LGTM!

Proper cleanup of event handlers to prevent memory leaks. The unsubscriptions correctly mirror the subscriptions in the constructor.

90-96: LGTM!

The exception handling approach is reasonable for non-critical status reporting. Logging at warning level ensures visibility without disrupting the main processing flow.

98-103: LGTM!

Method signatures simplified by removing the channel parameter. Status reporting is now handled internally via the event-driven pattern.

311-361: LGTM!

Status reporting consistently migrated to use HandleCompStatusEvent across all computer processing operations (availability, NetSessionEnum, NetWkstaUserEnum, RegistrySessions).

819-828: LGTM!

Hosting computer context correctly passed to cert abuse processor methods, enabling accurate status reporting for contacted machines.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/Runtime/ObjectProcessors.cs (1)
764-799: Inconsistent ObjectId in status events.

There's an inconsistency between the two status events in ProcessEnterpriseCA:

Line 769: Uses resolvedSearchResult.ObjectId (the CA's object ID)

Line 798: Uses sid (the hosting computer's SID)

Both events report the same task (ProcessEnterpriseCA) but with different ObjectId values. If both CertServices and CARegistry collection methods are enabled, this could cause confusion in status output.

Consider aligning the ObjectId values:
                        await HandleCompStatusEvent(new CSVComputerStatus
                        {
                            Status = ComputerStatus.Success,
                            ComputerName = resolvedSearchResult.DisplayName,
                            Task = nameof(ProcessEnterpriseCA),
-                            ObjectId = sid,
+                            ObjectId = resolvedSearchResult.ObjectId,
                        });

🧹 Nitpick comments (1)

src/Runtime/ObjectProcessors.cs (1)

756-830: Consider deduplicating host resolution logic.

If both CertServices and CARegistry collection methods are enabled, the same dnsHostName is resolved twice (lines 761-763 and 791-792), and ret.HostingComputer is set twice. This could be refactored to resolve once at the start.

One approach would be to hoist the host resolution outside of the flag checks:

        if (_methods.HasFlag(CollectionMethod.CertServices)) {
            var caName = entry.GetProperty(LDAPProperties.Name);
            var dnsHostName = entry.GetProperty(LDAPProperties.DNSHostName);

            if (caName != null && dnsHostName != null) {
-               if (await _context.LDAPUtils.ResolveHostToSid(dnsHostName, resolvedSearchResult.DomainSid) is
-                       (true, var sid) && sid.StartsWith("S-1-")) {
-                   ret.HostingComputer = sid;
                    // ... status event and enrollment processing
-               }
            }
        }

-       if (_methods.HasFlag(CollectionMethod.CARegistry)) {
+       if (_methods.HasFlag(CollectionMethod.CARegistry) && ret.HostingComputer == null) {
            // Only resolve if not already done above

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ed84a79 and e6da9e7.

📒 Files selected for processing (3)

src/Runtime/CollectionTask.cs (3 hunks)
src/Runtime/LDAPConsumer.cs (3 hunks)
src/Runtime/ObjectProcessors.cs (10 hunks)

🧰 Additional context used

🧬 Code graph analysis (3)

src/Runtime/LDAPConsumer.cs (1)

src/Runtime/ObjectProcessors.cs (3)

ObjectProcessors (21-939)

ObjectProcessors (45-71)

ClearEventHandlers (73-75)

src/Runtime/ObjectProcessors.cs (2)

src/Runtime/CollectionTask.cs (2)

Task (64-109)

Task (111-153)

src/Producers/ComputerFileProducer.cs (2)

Task (32-119)

Task (122-126)

src/Runtime/CollectionTask.cs (1)

src/Runtime/ObjectProcessors.cs (3)

ObjectProcessors (21-939)

ObjectProcessors (45-71)

ClearEventHandlers (73-75)

🔇 Additional comments (5)

src/Runtime/CollectionTask.cs (1)

114-114: LGTM - Event-driven refactor correctly applied.

The changes properly integrate with the new status reporting architecture:

Channel passed to constructor for internal event handling

ProcessObject signature updated (channel no longer passed per-call)

Event handlers cleared after processing completes to prevent leaks

Also applies to: 132-132, 149-150

src/Runtime/LDAPConsumer.cs (1)

21-21: LGTM - Consistent with CollectionTask.cs refactor.

The event-driven status reporting pattern is correctly applied here, matching the implementation in CollectionTask.cs.

Also applies to: 37-37, 54-55

src/Runtime/ObjectProcessors.cs (3)

43-75: LGTM - Clean event subscription pattern.

The constructor properly stores the channel and wires up the event handler. ClearEventHandlers() correctly unsubscribes to prevent memory leaks.

297-349: LGTM - Consistent status event reporting in computer processing.

All status events in ProcessComputerObject properly use HandleCompStatusEvent and maintain consistent structure with resolvedSearchResult.ObjectId.

77-83: Verify the event delegate type in SharpHoundCommonLib.Processors.CertAbuseProcessor.

The implementation correctly handles exceptions to prevent status reporting failures from crashing the processor. However, ensure that CertAbuseProcessor.ComputerStatusEvent supports async handlers (e.g., event EventHandler<CSVComputerStatus> where EventHandler is async-compatible), otherwise the async method may fire-and-forget. Since this type is from the external SharpHoundCommonLib package, check the library documentation or source to confirm the delegate signature.

src/Runtime/ObjectProcessors.cs

mykeelium

great work, I think this can be brought in. Could you also add the other processors delegate subscriptions in ObjectProcessors.cs?

mykeelium

Fantastic!

lrfalslev added 2 commits December 8, 2025 16:59

write to compStatus for CA lookups

92b38b3

update certAbuseProcessor call

e6da9e7

coderabbitai bot reviewed Dec 15, 2025

View reviewed changes

mykeelium reviewed Dec 15, 2025

View reviewed changes

src/Runtime/ObjectProcessors.cs Show resolved Hide resolved

mykeelium requested changes Dec 15, 2025

View reviewed changes

lrfalslev changed the title ~~Add CompStatus Events to CertAbuseProcessor~~ Add CompStatus Events to CertAbuseProcessor BED-6967 Dec 15, 2025

register compstatus event handler for all processors

6eb6525

lrfalslev requested a review from mykeelium December 17, 2025 17:59

mykeelium approved these changes Dec 17, 2025

View reviewed changes

update certAbuseProc dependency (#192)

179f88d

lrfalslev merged commit 70c2310 into 2.X Jan 16, 2026
3 checks passed

lrfalslev deleted the lfalslev/bed-6967 branch January 16, 2026 15:02

github-actions bot locked and limited conversation to collaborators Jan 16, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add CompStatus Events to CertAbuseProcessor BED-6967 #191

Add CompStatus Events to CertAbuseProcessor BED-6967 #191

lrfalslev commented Dec 15, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 15, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

mykeelium left a comment

Uh oh!

mykeelium left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add CompStatus Events to CertAbuseProcessor BED-6967 #191

Add CompStatus Events to CertAbuseProcessor BED-6967 #191

Conversation

lrfalslev commented Dec 15, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mykeelium left a comment

Choose a reason for hiding this comment

Uh oh!

mykeelium left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lrfalslev commented Dec 15, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 15, 2025 •

edited

Loading